-
Notifications
You must be signed in to change notification settings - Fork 646
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Make Script::fmt_asm a static method and add Script::str_asm #569
Conversation
src/blockdata/script.rs
Outdated
/// Get the assembly decoding of the script. | ||
pub fn asm(&self) -> String { | ||
/// Create an assembly decoding of the script in the given byte slice. | ||
pub fn str_asm(script: &[u8]) -> String { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would asm_from_bytes
be a better name? Both asm
and str_asm
return String
s.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm yeah naming. I figured not too important is't not really gonna be a high-usage method. asm
is a member method, that would be used more. I made str_asm
kinda cfr fmt_asm
, but asm_from_bytes
makes sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the good naming for the associated conversion function should include both original and return data type, like bytes_to_string
(str
is reserved for &str
return type). Thus, it will be used as Script::bytes_to_string
, which is very readable and explicit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I disagree on the return type being in the name, string
is as non-descriptive as it gets (could be hex for all we know), this should remain the chosen encoding, i.e. asm
. I'd be ok with bytes_to_asm
, but don't really care as long as it's descriptive.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree on bytes_to_asm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@stevenroose Would you be ok with giving this a more descriptive name? Rarely used or not, it is part of the public API we will be stuck with for a while.
src/blockdata/script.rs
Outdated
@@ -433,42 +439,42 @@ impl Script { | |||
} | |||
|
|||
/// Write the assembly decoding of the script to the formatter. | |||
pub fn fmt_asm(&self, f: &mut dyn fmt::Write) -> fmt::Result { | |||
pub fn fmt_asm(script: &[u8], f: &mut dyn fmt::Write) -> fmt::Result { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR could be backwards compatible if you renamed that fn to fmt_asm_bytes
(or similar) and implemented fmt_asm
based on that, like you did with asm
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is that worth the effort, you think? The fmt asm method is really already a helper that I don't think has any real users. I can wait until we do a breaking release for this to be included.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As it is this is a breaking change as far as I can tell since fmt_asm
is public. I think this little change could make this PR non-breaking, allowing us to change the milestone to 0.26.1. Afaik we wanted to get away from frequent API breaks, so there will probably be some 0.26.x releases to get new APIs out before any major refactoring in 0.27.0.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ack
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree with @sgeisler in his requested changes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK 2cb8185 . I have no strong opinions on naming, as 1) it is a debug/test method and 2) can be easily seen by looking up docs.
It will be a breaking change for rust-miniscript because we use this in our tests, but it should not be hard to fix.
This makes it convenient to print/construct the script assembly on byte slices withoout having to clone them to copy them to create a Script struct.
I changed the names. And rebased. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ack 851a3a1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, agree with this version
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tACK 851a3a1
Test log:
Mar 27 11:55:30.957 INFO testinator: Installing rust toolchain 'nightly'
Mar 27 11:55:49.972 INFO testinator: Installing rust toolchain 'stable'
Mar 27 11:56:54.896 INFO testinator: Installing rust toolchain '1.29.0'
Mar 27 11:56:55.916 INFO testinator: Preparing environment for rust nightly tests (9 configurations)
Mar 27 11:56:55.916 INFO testinator: Preparing environment for rust stable tests (8 configurations)
Mar 27 11:56:55.916 INFO testinator: Preparing environment for rust 1.29.0 tests (8 configurations)
Mar 27 11:59:19.513 INFO testinator: Running rust 1.29.0 tests in /tmp/rust-bitcoin-1.29.0.Cf3vnJcUsHEA/rust-bitcoin
Mar 27 11:59:19.513 DEBUG testinator: Generating lock file with rust=1.29.0
Mar 27 11:59:19.519 INFO testinator: Running rust nightly tests in /tmp/rust-bitcoin-nightly.o31ZRDPsAqge/rust-bitcoin
Mar 27 11:59:19.522 INFO testinator: Running rust stable tests in /tmp/rust-bitcoin-stable.e6g5fj0QAChO/rust-bitcoin
Mar 27 12:00:19.701 DEBUG testinator: Pinning cc to 1.0.41
Mar 27 12:00:20.015 DEBUG testinator: Pinning serde to 1.0.98
Mar 27 12:00:20.167 DEBUG testinator: Pinning serde_derive to 1.0.98
Mar 27 12:00:20.319 DEBUG testinator: Pinning byteorder to 1.3.4
Mar 27 12:01:18.231 INFO testinator: Test rust=nightly, features=[secp-recovery] succeeded!
Mar 27 12:01:25.878 INFO testinator: Test rust=stable, features=[secp-recovery] succeeded!
Mar 27 12:01:54.922 INFO testinator: Test rust=1.29.0, features=[secp-recovery] succeeded!
Mar 27 12:02:23.395 INFO testinator: Test rust=nightly, features=[use-serde] succeeded!
Mar 27 12:02:45.540 INFO testinator: Test rust=stable, features=[use-serde] succeeded!
Mar 27 12:02:52.858 INFO testinator: Test rust=nightly, features=[base64] succeeded!
Mar 27 12:03:17.262 INFO testinator: Test rust=nightly, features=[secp-lowmemory] succeeded!
Mar 27 12:03:17.285 INFO testinator: Test rust=stable, features=[base64] succeeded!
Mar 27 12:03:58.190 INFO testinator: Test rust=1.29.0, features=[use-serde] succeeded!
Mar 27 12:03:58.738 INFO testinator: Test rust=nightly, features=[rand] succeeded!
Mar 27 12:03:58.870 INFO testinator: Test rust=stable, features=[secp-lowmemory] succeeded!
Mar 27 12:04:31.496 INFO testinator: Test rust=nightly, features=[unstable] succeeded!
Mar 27 12:04:34.408 INFO testinator: Test rust=stable, features=[rand] succeeded!
Mar 27 12:04:50.245 INFO testinator: Test rust=1.29.0, features=[base64] succeeded!
Mar 27 12:05:01.139 INFO testinator: Test rust=nightly, features=[bitcoinconsensus] succeeded!
Mar 27 12:05:22.010 INFO testinator: Test rust=stable, features=[bitcoinconsensus] succeeded!
Mar 27 12:05:37.926 INFO testinator: Test rust=nightly, features=[secp-recovery,use-serde,base64,secp-lowmemory,rand,unstable,bitcoinconsensus] succeeded!
Mar 27 12:05:43.496 INFO testinator: Test rust=1.29.0, features=[secp-lowmemory] succeeded!
Mar 27 12:05:59.844 INFO testinator: Test rust=nightly, features=[] succeeded!
Mar 27 12:06:08.202 INFO testinator: Test rust=stable, features=[secp-recovery,use-serde,base64,secp-lowmemory,rand,bitcoinconsensus] succeeded!
Mar 27 12:06:30.850 INFO testinator: Test rust=1.29.0, features=[rand] succeeded!
Mar 27 12:06:33.799 INFO testinator: Test rust=stable, features=[] succeeded!
Mar 27 12:07:20.275 INFO testinator: Test rust=1.29.0, features=[bitcoinconsensus] succeeded!
Mar 27 12:08:10.412 INFO testinator: Test rust=1.29.0, features=[secp-recovery,use-serde,base64,secp-lowmemory,rand,bitcoinconsensus] succeeded!
Mar 27 12:08:52.316 INFO testinator: Test rust=1.29.0, features=[] succeeded!
Mar 27 12:08:56.460 INFO testinator: Fuzzing deserialize_script
Mar 27 12:10:10.226 INFO testinator: Successfully fuzzed deserialize_script
Mar 27 12:10:10.226 INFO testinator: Fuzzing uint128_fuzz
Mar 27 12:11:12.186 INFO testinator: Successfully fuzzed uint128_fuzz
Mar 27 12:11:12.186 INFO testinator: Fuzzing deserialize_amount
Mar 27 12:12:13.210 INFO testinator: Successfully fuzzed deserialize_amount
Mar 27 12:12:13.211 INFO testinator: Fuzzing deserialize_transaction
Mar 27 12:13:15.198 INFO testinator: Successfully fuzzed deserialize_transaction
Mar 27 12:13:15.198 INFO testinator: Fuzzing deser_net_msg
Mar 27 12:14:18.252 INFO testinator: Successfully fuzzed deser_net_msg
Mar 27 12:14:18.253 INFO testinator: Fuzzing deserialize_address
Mar 27 12:15:19.242 INFO testinator: Successfully fuzzed deserialize_address
Mar 27 12:15:19.242 INFO testinator: Fuzzing deserialize_block
Mar 27 12:16:21.104 INFO testinator: Successfully fuzzed deserialize_block
Mar 27 12:16:21.104 INFO testinator: Fuzzing outpoint_string
Mar 27 12:17:22.236 INFO testinator: Successfully fuzzed outpoint_string
Mar 27 12:17:22.236 INFO testinator: Fuzzing deserialize_psbt
Mar 27 12:18:25.179 INFO testinator: Successfully fuzzed deserialize_psbt
I also think this version isn't API breaking anymore, am I right? @stevenroose |
Make Script::fmt_asm a static method and add Script::str_asm
This makes it convenient to print/construct the script assembly on byte slices withoout having to clone them to copy them to create a Script struct.